Skip to content

Conversation

@iandmorris
Copy link
Contributor

An area of flash memory on the RA4M1 MCU is used to store information used to configure the device following a reset. This patch instructs the linker to reserve this memory area and provides kconfig options that are used to populate it (at build time) with the desired device configuration.

NOTES:

The only board that currently uses the RA4M1 soc port is the Ardunio UNO R4 Minima. This board uses a bootloader to load the application. As this bootloader is located in the lower part of the RA4M1 flash memory it handles the reservation (and configuration) of the option setting memory area. In this case (and others where the application does not reside in the area that contains the option setting memory) this functionality can be disabled via the OPTION_SETTING_MEMORY kconfig option, however leaving it enabled does not cause any issues other than wasting 256 bytes of flash memory.

This option MUST be enabled if the application resides in the area that contains the option setting memory (which is usually the case if a bootloader is not used) - otherwise unintended behavior can occur due to misconfiguration of the device.

Comment on lines 22 to 44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to Kconfig

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to the devicetree definitions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hoco: hoco {
compatible = "fixed-clock";
clock-frequency = <24000000>;
status = "okay";
#clock-cells = <0>;
};

Please use this definition for determining HOCO frequency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default n not needed

Comment on lines 27 to 34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should be dts entries

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

settings here should have SOC_ prefix

@iandmorris iandmorris force-pushed the ra_opt_set_mem branch 2 times, most recently from a20b1c7 to 33bb06c Compare April 5, 2024 01:23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't estimate what the design intention from the "Set all fields to 1” description.
Since WDTSTPCTL is 1, the watchdog will not start, so it would be better to state that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The voltage detecting and enabling HOCO on reset setting seems good to be managed by hwinfo driver.
If you are having difficulty creating a suitable devicetree at the moment,
I think it's okay to set a fixed value at this point.

@soburi
Copy link
Member

soburi commented Apr 6, 2024

@iandmorris

I have created PR #71174 based on these patches.
I notice that The option setting is a common function in the RA series.
So, I think defining in a standard file in the RA series is good.

@iandmorris iandmorris force-pushed the ra_opt_set_mem branch 2 times, most recently from 7426bdf to f8e5701 Compare April 20, 2024 01:39
Comment on lines 26 to 29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use the devicetree definition to determine hoco frequency by the following code.

DT_PROP(DT_PATH(clocks, hoco), clock_frequency)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @soburi, i'm new to zephyr so i could be wrong about this but is seems like I can't use:

DT_PROP(DT_PATH(clocks, hoco), clock_frequency)

inside a Kconfig file to access a devicetree property, however i can use this within a 'C' file to access a devicetree property.

when i check other Kconfig files that refer to the devicetree they all seem to use:

dt_node_xxx_yyy

see soc/nxp/mcx/mcxnx4x/Kconfig for an example

let me know what you think, I probably missed something obvious :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part ...

https://github.com/zephyrproject-rtos/zephyr/pull/71017/files#diff-7cd037a8cbd8c3e02942d0cbda9cf90aa08fbab8ab55dcf57b46e12f660662beR8-R18

can rewrite as follows.
We can refer the hoco clock frequency directly in code, so we no need to define hoco freq in Kconfig.

#define HOCO_FREQ DT_PROP(DT_PATH(clocks, hoco), clock_frequency)

#if HOCO_FREQ == 24000000
#define OFS1_HOCO_FREQ		0
#elif HOCO_FREQ == 32000000
#define OFS1_HOCO_FREQ		2
#elif HOCO_FREQ == 48000000
#define OFS1_HOCO_FREQ		4
#elif HOCO_FREQ == 64000000
#define OFS1_HOCO_FREQ		5
#else
#error "Unsupported HOCO frequency"
#endif

Copy link
Member

@soburi soburi Apr 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether the hoco is valid or invalid, I don't think there is any problem with judging it by the following code.

DT_NODE_HAS_STATUS(DT_PATH(clocks, hoco), okay)

If you are considering a case where the not enabled the hoco at reset but is enabled during startup processing, add a property to dts/bindings/clock/renesas,ra-clock-generation-circuit.yaml. I think it's also good. (But this might be overthinking.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is same as SOC_OPTION_SETTING_HOCO_FREQ.

https://github.com/zephyrproject-rtos/zephyr/pull/71017/files#diff-7cd037a8cbd8c3e02942d0cbda9cf90aa08fbab8ab55dcf57b46e12f660662beR122

This line can rewrite

		.HOCOEN = !DT_NODE_HAS_STATUS(DT_PATH(clocks, hoco), okay),

And we can remove this Kconfig option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to create dts/bindings/hwinfo/renesas,ra-lvd.yaml and define it in the devicetree node.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this setting needs to be done in this PR.
Since the hwinfo driver is used to obtain the startup factor after a reset, I think it would be a good idea to create a devicetree definition related to this and use that.
(If you are new to Zephyr development, creating a hwinfo driver brings good experience.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soburi, Agreed, I would like to implement this setting via the device tree but do it in another PR. Shall I leave the VDSEL as it is for this PR or maybe remove the option?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't leave options that don't work, as they confuse users.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OFS is used widely in the RA series.
It should place a common directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soburi, while it's true that the OFS is used in a lot of RA microcontrollers it seems the contents of the registers, and their memory location is different in pretty much every series. A quick check of the device datasheets shows that RA6 series has different OSF1 register contents than RA4 series, as does the RA0 series. Therefore, I think it is better to leave this where it is...

Copy link
Member

@soburi soburi May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, let's implement the individual implementation in soc.c.

@iandmorris iandmorris force-pushed the ra_opt_set_mem branch 2 times, most recently from eef6ecf to c5e15f7 Compare May 1, 2024 15:35
An area of flash memory on the RA4M1 MCU is used to store information
used to configure the device following a reset. This patch instructs
the linker to reserve this memory area and provides kconfig options
that are used to populate it (at build time) with the desired device
configuration.

Signed-off-by: Ian Morris <[email protected]>
@iandmorris
Copy link
Contributor Author

@soburi, i think i have addressed all the comments. let me know if i missed something or if you have anything further.

Copy link
Member

@soburi soburi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to make this OK for now.

Renesas has officially started supporting it, so there may be things to consider in the future.

#72150

@iandmorris
Copy link
Contributor Author

Hi @thedjnK, I think I have addressed your comments. When you get a minute could you take a look and approve (or let me know if further changes are required)?
Thanks
Ian

@iandmorris iandmorris requested a review from thedjnK May 5, 2024 01:59
Comment on lines +13 to +15
config SOC_OPTION_SETTING_MEMORY
bool "Option Setting Memory"
default y
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this have a description? Never used these parts but "option setting memory" to me has no meaning, I have no idea what it is or what disabling it would do. Non-blocking comment and can be updated in a future PR.

@fabiobaltieri fabiobaltieri merged commit 0fd5365 into zephyrproject-rtos:main May 6, 2024
@iandmorris iandmorris deleted the ra_opt_set_mem branch May 6, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants